Add retry logic with jitter for sub_issue_write to handle parallel calls#1843
Add retry logic with jitter for sub_issue_write to handle parallel calls#1843
Conversation
Co-authored-by: SamMorrowDrums <4811358+SamMorrowDrums@users.noreply.github.com>
Co-authored-by: SamMorrowDrums <4811358+SamMorrowDrums@users.noreply.github.com>
Co-authored-by: SamMorrowDrums <4811358+SamMorrowDrums@users.noreply.github.com>
Co-authored-by: SamMorrowDrums <4811358+SamMorrowDrums@users.noreply.github.com>
Co-authored-by: SamMorrowDrums <4811358+SamMorrowDrums@users.noreply.github.com>
Co-authored-by: SamMorrowDrums <4811358+SamMorrowDrums@users.noreply.github.com>
Co-authored-by: SamMorrowDrums <4811358+SamMorrowDrums@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR adds exponential backoff with random jitter to the sub_issue_write tool to handle parallel API calls that fail due to GitHub's sequential priority assignment. AI agents frequently call this tool in parallel when adding multiple sub-issues, causing 422 "Priority has already been taken" errors.
Changes:
- Implemented retry logic with up to 3 attempts for priority conflict errors (422 status with specific message)
- Added 50-200ms random jitter between retries using
math/rand/v2.IntN()for desynchronization - Added comprehensive test coverage with 4 scenarios: successful retry, exhausted retries, non-retryable errors, and immediate success
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/github/issues.go | Refactored AddSubIssue function to implement retry logic with jitter, improved error handling, and response body management |
| pkg/github/issues_test.go | Added Test_AddSubIssue_RetryLogic with 4 test cases covering retry scenarios using atomic counters for thread-safe mock handlers |
| if err == nil && resp.StatusCode == http.StatusCreated { | ||
| r, err := json.Marshal(subIssue) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to marshal response: %w", err) | ||
| } | ||
| return utils.NewToolResultText(string(r)), nil | ||
| } |
There was a problem hiding this comment.
Potential resource leak: The response body is not closed in the success path. The original code had a deferred close for the response body, but in the retry logic, if the request succeeds, the response body should still be closed to prevent resource leaks. Consider adding _ = resp.Body.Close() before returning on success.
There was a problem hiding this comment.
Fixed in 8e0a81f. Added _ = resp.Body.Close() before returning in the success path to prevent the resource leak.
Co-authored-by: SamMorrowDrums <4811358+SamMorrowDrums@users.noreply.github.com>
Summary
Adds exponential backoff with random jitter to
sub_issue_writeoperations that fail with priority conflicts when called in parallel.Why
AI agents frequently call
sub_issue_writein parallel when adding multiple sub-issues to an epic. GitHub's API assigns priority sequentially, causing parallel calls to fail with422 Priority has already been takenerrors.What changed
AddSubIssueto retry up to 3 times on priority conflict errors (422 with "Priority has already been taken")math/rand/v2.IntN()(thread-safe, auto-seeded)Test_AddSubIssue_RetryLogicwith 4 test cases covering success/failure/non-retryable scenariosatomic.Int32for thread-safe test countersMCP impact
sub_issue_writewithaddmethod now auto-retries priority conflicts. No schema changes.Prompts tested (tool changes only)
Security / limits
Tool renaming
Lint & tests
./script/lint./script/testDocs
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.